Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/file context duplicate entry check #183

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

gdesrosi
Copy link
Contributor

Adding check “E-010” to check for multiple specification of the same entry which setfiles
would report as an error during the policy build. Even if those two entries were identical,
setfiles would still report that as an error during the policy build. For instance,
'/foo – foo_context' and '/foo – foo_context' would be considered multiple specification
for the same entry just like '/foo – foo_context' and '/foo – bar_context' would be
considered multiple specification. This check helps detecting that kind of problem by
parsing all the fc entries and looking for multiple specifications across all other file
contexts instead of just comparing entries within each file since it is possible to have
multiple specifications across file contexts and that would also cause issues.

All findings are returned as selint errors with the file context names and line numbers
involved in the result of the violation.

Other changes to support addition of file context duplicate entry check as well, which include:

1)Addition of an fc_entry map to map.c to store all the fc entries
  across every file context.

2)Addition of the fc_entry_map_info and fc_entry_hash structs to
  store the relevant information into that map that will later be
  used in the file context duplicate entry check.

3)Modification of the fc_entry struct to take into account when an
  entry is within and ifdef/ifndef macro.

4)Modification of the parse_fc_file method to parse and store ifdef/ifndef
  definitions to relevant fc entry as well as storing those entry into the
  fc_entry_map which again will be used later in the file context
  duplicate entry check.

@gdesrosi gdesrosi force-pushed the feature/file-context-duplicate-entry-check branch 3 times, most recently from 0c29cb5 to 412a52a Compare December 12, 2020 05:38
@@ -67,6 +67,7 @@ enum error_ids {
E_ID_UNKNOWN_PERM = 7,
E_ID_UNKNOWN_CLASS = 8,
E_ID_EMPTY_BLOCK = 9,
E_ID_FC_DUPLICATE_ENTRY = 10,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just merged #179 which also added an error check and took E-010. Can you switch to E-011, please?

static bool is_same_fc_entry(const struct fc_entry *entry_one,
const struct fc_entry *entry_two) {

return !strcmp(entry_one->path, entry_two->path)
Copy link
Member

@dburgener dburgener Dec 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a ton of indentation. I get that you're trying to make the complicated block clear, but can you cut back to one tab at each level instead of two?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dburgener I will address all the formatting issues that you've pointed out, but I had a question. So I'm using the eclipse c/c++ plugin and generally when I'm done with a function i use their default formatting template and I know it does all sorts of things like limiting number of letters per line, tab formatting and what not. I was wondering if you were using any sort of formatting template to format your code before submitting PRs or if there is a particular guideline that you use that maybe I can modify the formatting template to follow.

src/fc_checks.c Outdated
node->data.fc_data->path);
struct check_result *ret = NULL;

/***********************************************************************************
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than the comment, just do:

if(!out) {
    alloc_internal_error("message here");
}

src/fc_checks.c Outdated
* 2)two duplicates whereby one of them is within an ifdef/ifndef and the other *
* one is not within any. *
* *
* 3)two two duplicates whereby both are within the same ifdef/ifndef. by both *
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

repeated word

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And "by" is the first word in a sentence and not capitalized.

src/fc_checks.c Outdated
* here I'm not referring to literally be defined under the same conditional *
* although it would also return true in this case, rather I'm referring to the *
* parameters or conditions being the same. so if you had the same specification *
* under distro_redhat in two different files,it would treat them as being under *
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing space in "files,it"

src/fc_checks.c Outdated
} else if (entry_one->conditional && entry_two->conditional) {
if (entry_one->conditional->flavor == entry_two->conditional->flavor
&& (!strcmp(entry_one->conditional->condition,
entry_two->conditional->condition)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please align multiple arguments to a function call together. You can tab up to the previous indentation and then use spaces for alignment

src/maps.c Outdated
entry = malloc(sizeof(struct fc_entry_hash));
entry->key = strdup(info->entry->path);
entry->val = info;
HASH_ADD_KEYPTR(hh_fc_entry, fcs_entry_map, entry->key, strlen(entry->key), entry);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spaces instead of tabs

src/maps.c Outdated
entry->val = info;
HASH_ADD_KEYPTR(hh_fc_entry, fcs_entry_map, entry->key, strlen(entry->key), entry);
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

eliminate both blank lines here please

Copy link
Contributor

@cgzones cgzones left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some memory leaks in the test suite:

diff --git a/tests/check_fc_checks.c b/tests/check_fc_checks.c
index c61953c..8d6461b 100644
--- a/tests/check_fc_checks.c
+++ b/tests/check_fc_checks.c
@@ -422,9 +422,11 @@ START_TEST (test_check_file_contexts_duplicate_entry){
 	 **************************************************/
 	node_entry->conditional = malloc(sizeof(struct conditional_data));
 	memset(node_entry->conditional, 0, sizeof(struct conditional_data));
+	free(node_entry->conditional->condition);
 	node_entry->conditional->condition = strdup("distro_gentoo");
 	node_entry->conditional->flavor = CONDITION_IFDEF;
 
+	free_check_result(res);
 	res = check_file_contexts_duplicate_entry(data, node);
 	ck_assert_ptr_nonnull(res);
 	ck_assert_int_eq(res->severity, 'E');
@@ -437,6 +439,7 @@ START_TEST (test_check_file_contexts_duplicate_entry){
 	 **************************************************/
 	node_entry->conditional->flavor = CONDITION_IFNDEF;
 
+	free_check_result(res);
 	res = check_file_contexts_duplicate_entry(data, node);
 	ck_assert_ptr_nonnull(res);
 	ck_assert_int_eq(res->severity, 'E');
@@ -454,6 +457,7 @@ START_TEST (test_check_file_contexts_duplicate_entry){
 	map_entry->conditional->condition = strdup("distro_gentoo");
 	node_entry->conditional->flavor = CONDITION_IFDEF;
 
+	free_check_result(res);
 	res = check_file_contexts_duplicate_entry(data, node);
 	ck_assert_ptr_nonnull(res);
 	ck_assert_int_eq(res->severity, 'E');
@@ -467,6 +471,7 @@ START_TEST (test_check_file_contexts_duplicate_entry){
 	node_entry->conditional->flavor = CONDITION_IFNDEF;
 	map_entry->conditional->flavor = CONDITION_IFNDEF;
 
+	free_check_result(res);
 	res = check_file_contexts_duplicate_entry(data, node);
 	ck_assert_ptr_nonnull(res);
 	ck_assert_int_eq(res->severity, 'E');
@@ -480,6 +485,7 @@ START_TEST (test_check_file_contexts_duplicate_entry){
 	 **************************************************/
 	node_entry->conditional->flavor = CONDITION_IFDEF;
 
+	free_check_result(res);
 	res = check_file_contexts_duplicate_entry(data, node);
 	ck_assert_ptr_null(res);
 
@@ -488,6 +494,7 @@ START_TEST (test_check_file_contexts_duplicate_entry){
 	 * and the other one within ifndef and the        *
 	 * conditions differ                              *
 	 **************************************************/
+	free(node_entry->conditional->condition);
 	node_entry->conditional->condition = strdup("distro_redhat");
 
 	res = check_file_contexts_duplicate_entry(data, node);
@@ -502,6 +509,7 @@ START_TEST (test_check_file_contexts_duplicate_entry){
 	 **************************************************/
 	map_entry->conditional->flavor = CONDITION_IFDEF;
 
+	free_check_result(res);
 	res = check_file_contexts_duplicate_entry(data, node);
 	ck_assert_ptr_null(res);
 
@@ -522,8 +530,10 @@ START_TEST (test_check_file_contexts_duplicate_entry){
 	 * checking to find problematic duplicate          *
 	 * across different file contexts.                 *
 	 **************************************************/
+	free(data->filename);
 	data->filename = strdup("bar.fc");
 
+	free_check_result(res);
 	res = check_file_contexts_duplicate_entry(data, node);
 	ck_assert_ptr_nonnull(res);
 	ck_assert_int_eq(res->severity, 'E');
@@ -531,8 +541,10 @@ START_TEST (test_check_file_contexts_duplicate_entry){
 	ck_assert_ptr_nonnull(res->message);
 
 	free_check_result(res);
+	free(data->filename);
 	free(data);
 	free_policy_node(node);
+	free_fc_entry(map_entry);
 	free_all_maps();
 }
 END_TEST

src/parse_fc.c Outdated
token = strtok(NULL, "`");
token = strtok(token, "'");
ifdef_condition = malloc(strlen(token) + 1);
strcpy(ifdef_condition, token);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe use strdup? (strcpy smells)

@gdesrosi
Copy link
Contributor Author

gdesrosi commented Dec 13, 2020

@cgzones yes I'm aware of the memory leaks. I actually fixed those last night, but I'm waiting to get those code review comments fix addressed so i can do another push with everything included

@gdesrosi
Copy link
Contributor Author

gdesrosi commented Dec 13, 2020

@dburgener in the free_check_result function there is no NULL check on res->message before attempting to free it. I'm wondering if there is a specific reason for that since we seem to consistently do that pretty much everywhere else. I ran into issues last night working on the test suite with that because i mistakenly called free_check_result on a NULL check_result, but then I thought it should have still checked that this was a valid pointer before attempting to free it, ignore it it it isn't, or error out with a message indicating that you are trying to free an invalid pointer.

@dburgener
Copy link
Member

@gdesrosi The lack of a NULL check on res is an oversight, feel free to add it (and that's what I presume you're hitting). The lack of NULL check on res->message is intentional, since free(NULL); is a no-op.

@gdesrosi
Copy link
Contributor Author

@dburgener yeah you're right the issue is res. I can fix that as part of this PR, but I was thinking maybe for traceability you may have wanted that to be done separately? It's small enough that it probably is not necessary to open a PR for that. I also asked you a question regarding whether or not we use formatting template for selint, or if we even have specific guidelines that we use that maybe I can use to tweak my eclipse so that it follows those. I'm not sure if you saw that already.

@dburgener
Copy link
Member

I can fix that as part of this PR, but I was thinking maybe for traceability you may have wanted that to be done separately? It's small enough that it probably is not necessary to open a PR for that.

A separate commit would be good. Nothing is too small for its own PR, although I'm fine with putting this in its own commit in this PR.

I was wondering if you were using any sort of formatting template to format your code before submitting PRs or if there is a particular guideline that you use that maybe I can modify the formatting template to follow.

Unfortunately nothing documented. I'd like to run an automated checker, but unfortunately last time I looked I couldn't find one that did what I want. The main things I see in this PR are indentation issues. We want tabs for indentation and spaces for alignment (tab to the indent level then space to the alignment point). One tab per indentation level. Align multiple boolean expressions so its clear where the nesting is.

The best general rule I can give you is to try and follow the style the code is already doing, but I recognize that that's less helpful than a tool or document.

@gdesrosi gdesrosi force-pushed the feature/file-context-duplicate-entry-check branch 5 times, most recently from a149031 to a06992d Compare December 14, 2020 04:41
@gdesrosi
Copy link
Contributor Author

@dburgener @stevedlawrence @cgzones Does anyone know what could cause "make check-valgrind" to not return any memory leaks for me locally, but failing on the automated PR checker?

@stevedlawrence
Copy link
Member

stevedlawrence commented Dec 14, 2020

Seems to have to do with the options you provide when you run ./configure. The github actions runs the following when configuring with gcc:

./configure --enable-werror CFLAGS="-Wno-error=conversion -Wno-error=sign-conversion --coverage"

With my usual ./configure options I wasn't getting any errors like you, but with the above I can dupliate the leak.

@gdesrosi
Copy link
Contributor Author

Thanks @stevedlawrence

src/maps.c Outdated
@@ -27,6 +27,7 @@ static struct hash_elem *perm_map = NULL;
static struct hash_elem *mods_map = NULL;
static struct hash_elem *mod_layers_map = NULL;
static struct hash_elem *ifs_map = NULL;
static struct fc_entry_hash *fcs_entry_map = NULL;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I'm just missing something, but is there a reason this is named "fcs_entry" instead of "fc_entries"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No reason other than me being bad at my variable naming. Well I guess I can also blame that on English not being my first language lol

src/fc_checks.c Outdated
* one is not within any. *
* *
* 3)Two duplicates whereby both are within the same ifdef/ifndef. By both *
* here I'm not referring to literally be defined under the same conditional *
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you reword to eliminate the use of first person singular in comments?

src/parse_fc.c Outdated
token = strtok(line, "`");
if (token) {
token = strtok(NULL, "`");
token = strtok(token, "'");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be strtok(NULL, "'");" since you're continuing in the same string?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes that is a mistake.

src/parse_fc.c Outdated
token = strtok(line, "`");
if (token) {
token = strtok(NULL, "`");
token = strtok(token, "'");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question as above

ifndef_condition = strdup(token);
}
continue;
} else if (!strncmp(line, "')", 2)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The lack of durability here scares me. Ending an ifdef block with eg ' ) (note space) would be legal, and missed by this.

The durable solution is probably a fairly large and high risk change, so maybe for now, just a TODO comment noting the issue? SELint does generally assume reasonable style, but this one seems pretty annoying if it were to hit.

ifndef_condition = NULL;
}
continue;
} else if (!strncmp(line, "', `", 4) || !strncmp(line, "',`", 3)) { // Skip over m4 constructs
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks broken. This would typically flip the ifdef. These are for situations like:

ifdef(`foo',`
/some/path label1
',`
/some/path label2
')

In order to assign label1 if foo was set and label2 if it's not.  If I'm reading this right, this would treat both as being in the ifdef and result in a false positive in this scenario.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that is correct. I failed to take this situation into account. so I'll need to rework this

src/parse_fc.c Outdated
}

struct fc_entry *entry = parse_fc_line(line, conditional);
free(conditional);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given that you're freeing it immediately after the call, why not allocate on the stack rather than the heap?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On second thought, you allocate a copy of it in parse_fc_line(). Why not allocate it here and then take ownership of that copy in parse_fc_line?

src/parse_fc.c Outdated
} else if (is_within_ifndef) {
conditional = malloc(sizeof(struct conditional_data));
conditional->flavor = CONDITION_IFNDEF;
conditional->condition = strdup(ifndef_condition);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These strings will get leaked if there is a parse error in the line (the fc_entry node will never take ownership of them.

You either need to check for (and free) them in the cleanup tag in parse_fc_line(), or sidestep all the memory management issues here by just passing the flavor and string to parse_fc_line directly.

src/parse_fc.c Outdated
if (token) {
token = strtok(NULL, "`");
token = strtok(token, "'");
ifdef_condition = malloc(strlen(token) + 1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for malloc now that you're strduping. malloc will leak memory now.

…ich include:

    1)Addition of an fc_entry map to map.c to store all the fc entries
      across every file context.

    2)Addition of the fc_entry_map_info and fc_entry_hash structs to
      store the relevant information into that map that will later be
      used in the file context duplicate entry check.

    3)Modification of the fc_entry struct to take into account when an
      entry is within and ifdef/ifndef macro.

    4)Modification of the parse_fc_file method to parse and store ifdef/ifndef
      definitions to relevant fc entry as well as storing those entry into the
      fc_entry_map which again will be used later in the file context
      duplicate entry check.
…entry which setfiles

would report as an error during the policy build. Even if those two entries were identical,
setfiles would still report that as an error during the policy build. For instance,
'/foo – foo_context' and '/foo – foo_context' would be considered multiple specification
for the same entry just like '/foo – foo_context' and '/foo – bar_context' would be
considered multiple specification. This check helps detecting that kind of problem by
parsing all the fc entries and looking for multiple specifications across all other file
contexts instead of just comparing entries within each file since it is possible to have
multiple specifications across file contexts and that would also cause issues.

All findings are returned as selint errors with the file context names and line numbers
involve in the result of the violation.
@gdesrosi gdesrosi force-pushed the feature/file-context-duplicate-entry-check branch from a06992d to 0fdb8ff Compare December 15, 2020 04:23
@cgzones
Copy link
Contributor

cgzones commented Dec 15, 2020

Got crashes with clang sanitizers with the functional bats tests:

typfc_checks.c:248:16: runtime error: null pointer passed as argument 2, which is declared to never be null /usr/include/string.h:138:33: note: nonnull attribute specified here SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior fc_checks.c:248:16 in AddressSanitizer:DEADLYSIGNAL =================================================================
==12920==ERROR:
AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x0000004325e5 bp 0x7ffd4d3b9740 sp 0x7ffd4d3b8ee0 T0)
==12920==The signal is caused by a READ memory access.
==12920==Hint: address points to the zero page. #0 0x4325e5 in strcmp (/home/christian/Coding/workspaces/selint/src/selint+0x4325e5)
 #1 0x60e1a5 in is_same_fc_entry /home/christian/Coding/workspaces/selint/src/fc_checks.c:247:9
 #2 0x60ce27 in check_file_contexts_duplicate_entry /home/christian/Coding/workspaces/selint/src/fc_checks.c:376:8
 #3 0x608c72 in call_checks_for_node_type /home/christian/Coding/workspaces/selint/src/check_hooks.c:77:30
 #4 0x608a56 in call_checks /home/christian/Coding/workspaces/selint/src/check_hooks.c:62:9
 #5 0x602887 in run_checks_on_one_file /home/christian/Coding/workspaces/selint/src/runner.c:350:27
 #6 0x602c92 in run_all_checks /home/christian/Coding/workspaces/selint/src/runner.c:391:4
 #7 0x603307 in run_analysis /home/christian/Coding/workspaces/selint/src/runner.c:477:8
 #8 0x4cbc99 in main /home/christian/Coding/workspaces/selint/src/main.c:597:26
 #9 0x7fd288442d09 in __libc_start_main csu/../csu/libc-start.c:308:16
 #10 0x41f3e9 in _start (/home/christian/Coding/workspaces/selint/src/selint+0x41f3e9)
AddressSanitizer can not provide additional info. 
SUMMARY: AddressSanitizer: SEGV (/home/christian/Coding/workspaces/selint/src/selint+0x4325e5) in strcmp
==12920==ABORTING
typfc_checks.c:284:42: runtime error: null pointer passed as argument 2, which is declared to never be null /usr/include/string.h:138:33: note: nonnull attribute specified here
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior fc_checks.c:284:42 in AddressSanitizer:
DEADLYSIGNAL =================================================================
==16367==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x0000004325e5 bp 0x7ffc5307b810 sp 0x7ffc5307afb0 T0)
==16367==The signal is caused by a READ memory access.
==16367==Hint: address points to the zero page. #0 0x4325e5 in strcmp (/home/christian/Coding/workspaces/selint/src/selint+0x4325e5)
 #1 0x60fdf9 in is_multiple_fc_entry_spec /home/christian/Coding/workspaces/selint/src/fc_checks.c:284:8
 #2 0x60d05e in check_file_contexts_duplicate_entry /home/christian/Coding/workspaces/selint/src/fc_checks.c:404:15
 #3 0x608c72 in call_checks_for_node_type /home/christian/Coding/workspaces/selint/src/check_hooks.c:77:30
 #4 0x608a56 in call_checks /home/christian/Coding/workspaces/selint/src/check_hooks.c:62:9
 #5 0x602887 in run_checks_on_one_file /home/christian/Coding/workspaces/selint/src/runner.c:350:27
 #6 0x602c92 in run_all_checks /home/christian/Coding/workspaces/selint/src/runner.c:391:4
 #7 0x603307 in run_analysis /home/christian/Coding/workspaces/selint/src/runner.c:477:8
 #8 0x4cbc99 in main /home/christian/Coding/workspaces/selint/src/main.c:597:26
 #9 0x7f495c3d0d09 in __libc_start_main csu/../csu/libc-start.c:308:16
 #10 0x41f3e9 in _start (/home/christian/Coding/workspaces/selint/src/selint+0x41f3e9)
AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV (/home/christian/Coding/workspaces/selint/src/selint+0x4325e5) in strcmp
==16367==ABORTING

suggested change:

diff --git a/src/fc_checks.c b/src/fc_checks.c
index eb726bf..d37cdf6 100644
--- a/src/fc_checks.c
+++ b/src/fc_checks.c
@@ -233,6 +233,19 @@ struct check_result *check_file_context_types_exist(__attribute__((unused)) cons
        return NULL;
 }
 
+static bool null_str_eq(const char *a, const char *b)
+{
+       if (a == b) {
+               return true;
+       }
+
+       if (!a || !b) {
+               return false;
+       }
+
+       return strcmp(a, b) == 0;
+}
+
 /**********************************
  * Return true if the two fc_entry nodes are the same
  * and false otherwise
@@ -243,15 +256,15 @@ static bool is_same_fc_entry(const struct fc_entry *entry_one,
        return !strcmp(entry_one->path, entry_two->path)
                && ((!entry_one->context && !entry_two->context)        //when <<none>>
                        || (entry_one->obj == entry_two->obj
-                               && !strcmp(entry_one->path, entry_two->path)
-                               && !strcmp(entry_one->context->range,
-                                          entry_two->context->range)
-                               && !strcmp(entry_one->context->role,
-                                          entry_two->context->role)
-                               && !strcmp(entry_one->context->type,
-                                          entry_two->context->type)
-                               && !strcmp(entry_one->context->user,
-                                          entry_two->context->user)));
+                               && null_str_eq(entry_one->path, entry_two->path)
+                               && null_str_eq(entry_one->context->range,
+                                              entry_two->context->range)
+                               && null_str_eq(entry_one->context->role,
+                                              entry_two->context->role)
+                               && null_str_eq(entry_one->context->type,
+                                              entry_two->context->type)
+                               && null_str_eq(entry_one->context->user,
+                                              entry_two->context->user)));
 }
 
 /**********************************
@@ -268,13 +281,13 @@ static bool is_multiple_fc_entry_spec(const struct fc_entry *entry_one,
                return !strcmp(entry_one->path, entry_two->path)
                        && entry_one->obj == entry_two->obj
                        && !strcmp(entry_one->path, entry_two->path)
-                       && (strcmp(entry_one->context->range, entry_two->context->range)
-                               || strcmp(entry_one->context->role,
-                                         entry_two->context->role)
-                               || strcmp(entry_one->context->type,
-                                         entry_two->context->type)
-                               || strcmp(entry_one->context->user,
-                                         entry_two->context->user));
+                       && (!null_str_eq(entry_one->context->range, entry_two->context->range)
+                               || !null_str_eq(entry_one->context->role,
+                                               entry_two->context->role)
+                               || !null_str_eq(entry_one->context->type,
+                                               entry_two->context->type)
+                               || !null_str_eq(entry_one->context->user,
+                                               entry_two->context->user));
        }
 }
 
diff --git a/tests/functional/end-to-end.bats b/tests/functional/end-to-end.bats
index b66cf5a..a9a5b12 100644
--- a/tests/functional/end-to-end.bats
+++ b/tests/functional/end-to-end.bats
@@ -312,6 +312,7 @@ test_parse_error_impl() {
 
 @test "Enable/disable" {
        run ${SELINT_PATH} -c configs/empty.conf -e W-002 -e W-003 -d S-002 -d C-002 -r -s policies/check_triggers
+       echo ${output}
        [ "$status" -eq 0 ]
        count=$(echo ${output} | grep -o "S-002" | wc -l)
        [ "$count" -eq 0 ]

@gdesrosi
Copy link
Contributor Author

@cgzones Im still trying to figure out how to configure my environment so that it runs all the checks that the automated PR checker checks for. I tried running configure with those flags ./configure --enable-werror CFLAGS="-Wno-error=conversion -Wno-error=sign-conversion --coverage" which @stevedlawrence had suggested yesterday but I was getting build error with those flags on lex even on a clean checkout of master. I emailed @dburgener about that yesterday,but since I finished addressing those code review comments from yesterday I did a push, but I knew there were likely issues that the automated PR checker would detect.

@cgzones
Copy link
Contributor

cgzones commented Dec 15, 2020

./configure --enable-werror CFLAGS="-Wno-error=conversion -Wno-error=sign-conversion --coverage"
make check -j

works fine for me on Debian sid.
Can you share your build failures?

There a re actually two testsuites:
I. a libcheck based (tests/check_*.c), run by make check
II. a bats based (tests/functional/end-to-end.bats), run by cd tests/functional/ && bats end-to-end.bats

@gdesrosi
Copy link
Contributor Author

@cgzones here is what I'm seeing. I'm running on a centos8

make all-recursive
make[1]: Entering directory '/home/gdesrosiers/SELINT_COPY/selint'
Making all in src
make[2]: Entering directory '/home/gdesrosiers/SELINT_COPY/selint/src'
make all-am
make[3]: Entering directory '/home/gdesrosiers/SELINT_COPY/selint/src'
depbase=echo lex.o | sed 's|[^/]*$|.deps/&|;s|\.o$||';
gcc -DHAVE_CONFIG_H -I. -I.. -Wall -Wextra -Wcast-qual -Wconversion -Wmissing-format-attribute -Wmissing-noreturn -Wmissing-prototypes -Wpointer-arith -Wshadow -Wstrict-prototypes -Wundef -Wunused -Wwrite-strings -Werror -DSYSCONFDIR='"/usr/local/etc"' -Wno-error=conversion -Wno-error=sign-conversion --coverage -MT lex.o -MD -MP -MF $depbase.Tpo -c -o lex.o lex.c &&
mv -f $depbase.Tpo $depbase.Po
lex.c: In function ‘yylex’:
lex.c:1203:23: error: comparison of integer expressions of different signedness: ‘yy_size_t’ {aka ‘long unsigned int’} and ‘int’ [-Werror=sign-compare]
for ( yyl = 0; yyl < yyleng; ++yyl )
^
lex.c: In function ‘yy_get_next_buffer’:
lex.c:1867:42: warning: conversion to ‘yy_size_t’ {aka ‘long unsigned int’} from ‘int’ may change the sign of the result [-Wsign-conversion]
YY_CURRENT_BUFFER_LVALUE->yy_buf_size - number_to_move - 1;
^
lex.c:294:34: warning: conversion from ‘yy_size_t’ {aka ‘long unsigned int’} to ‘int’ may change value [-Wconversion]
#define YY_CURRENT_BUFFER_LVALUE yyg->yy_buffer_stack[yyg->yy_buffer_stack_top]
^~~
lex.c:1867:4: note: in expansion of macro ‘YY_CURRENT_BUFFER_LVALUE’
YY_CURRENT_BUFFER_LVALUE->yy_buf_size - number_to_move - 1;
^~~~~~~~~~~~~~~~~~~~~~~~
lex.c:1901:56: warning: conversion to ‘yy_size_t’ {aka ‘long unsigned int’} from ‘int’ may change the sign of the result [-Wsign-conversion]
num_to_read = YY_CURRENT_BUFFER_LVALUE->yy_buf_size -
^
lex.c:294:34: warning: conversion from ‘yy_size_t’ {aka ‘long unsigned int’} to ‘int’ may change value [-Wconversion]
#define YY_CURRENT_BUFFER_LVALUE yyg->yy_buffer_stack[yyg->yy_buffer_stack_top]
^~~
lex.c:1901:18: note: in expansion of macro ‘YY_CURRENT_BUFFER_LVALUE’
num_to_read = YY_CURRENT_BUFFER_LVALUE->yy_buf_size -
^~~~~~~~~~~~~~~~~~~~~~~~
lex.c:1935:29: warning: conversion to ‘yy_size_t’ {aka ‘long unsigned int’} from ‘int’ may change the sign of the result [-Wsign-conversion]
if ((int) (yyg->yy_n_chars + number_to_move) > YY_CURRENT_BUFFER_LVALUE->yy_buf_size) {
^
lex.c:1937:34: warning: conversion to ‘yy_size_t’ {aka ‘long unsigned int’} from ‘int’ may change the sign of the result [-Wsign-conversion]
int new_size = yyg->yy_n_chars + number_to_move + (yyg->yy_n_chars >> 1);
^
lex.c:1937:51: warning: conversion to ‘yy_size_t’ {aka ‘long unsigned int’} from ‘int’ may change the sign of the result [-Wsign-conversion]
int new_size = yyg->yy_n_chars + number_to_move + (yyg->yy_n_chars >> 1);
^
lex.c:1937:18: warning: conversion from ‘yy_size_t’ {aka ‘long unsigned int’} to ‘int’ may change value [-Wconversion]
int new_size = yyg->yy_n_chars + number_to_move + (yyg->yy_n_chars >> 1);
^~~
lex.c:1943:18: warning: conversion to ‘yy_size_t’ {aka ‘long unsigned int’} from ‘int’ may change the sign of the result [-Wsign-conversion]
yyg->yy_n_chars += number_to_move;
^~
lex.c:1943:21: warning: conversion from ‘yy_size_t’ {aka ‘long unsigned int’} to ‘int’ may change value [-Wconversion]
yyg->yy_n_chars += number_to_move;
^~~~~~~~~~~~~~
lex.c: In function ‘yyensure_buffer_stack’:
lex.c:2339:23: warning: conversion to ‘long unsigned int’ from ‘int’ may change the sign of the result [-Wsign-conversion]
(num_to_alloc * sizeof(struct yy_buffer_state*)
^
lex.c:2344:48: warning: conversion to ‘long unsigned int’ from ‘int’ may change the sign of the result [-Wsign-conversion]
memset(yyg->yy_buffer_stack, 0, num_to_alloc * sizeof(struct yy_buffer_state*));
^
lex.c:2346:30: warning: conversion to ‘size_t’ {aka ‘long unsigned int’} from ‘int’ may change the sign of the result [-Wsign-conversion]
yyg->yy_buffer_stack_max = num_to_alloc;
^~~~~~~~~~~~
lex.c:2356:18: warning: conversion from ‘long unsigned int’ to ‘int’ may change value [-Wconversion]
num_to_alloc = yyg->yy_buffer_stack_max + grow_size;
^~~
lex.c:2359:22: warning: conversion to ‘long unsigned int’ from ‘int’ may change the sign of the result [-Wsign-conversion]
num_to_alloc * sizeof(struct yy_buffer_state*)
^
lex.c:2366:30: warning: conversion to ‘size_t’ {aka ‘long unsigned int’} from ‘int’ may change the sign of the result [-Wsign-conversion]
yyg->yy_buffer_stack_max = num_to_alloc;
^~~~~~~~~~~~

@dburgener
Copy link
Member

dburgener commented Dec 15, 2020

I think this is your problem:

lex.c:1203:23: error: comparison of integer expressions of different signedness: ‘yy_size_t’ {aka ‘long unsigned int’} and ‘int’ [-Werror=sign-compare]

The rest of the warnings do seem to be getting treated as (EDIT: NOT) errors. Possibly an older version of lex than we've seen?

Try adding -Wno-error=sign-compare in your ./configure with the other -Wno-error flags and see if that resolves it. You should expect to still see the warnings, but they shouldn't break your build.

@gdesrosi
Copy link
Contributor Author

@cgzones what are you running bats with to get output like this?

==16367==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x0000004325e5 bp 0x7ffc5307b810 sp 0x7ffc5307afb0 T0)
==16367==The signal is caused by a READ memory access.
==16367==Hint: address points to the zero page. #0 0x4325e5 in strcmp (/home/christian/Coding/workspaces/selint/src/selint+0x4325e5)
#1 0x60fdf9 in is_multiple_fc_entry_spec /home/christian/Coding/workspaces/selint/src/fc_checks.c:284:8
#2 0x60d05e in check_file_contexts_duplicate_entry /home/christian/Coding/workspaces/selint/src/fc_checks.c:404:15
#3 0x608c72 in call_checks_for_node_type /home/christian/Coding/workspaces/selint/src/check_hooks.c:77:30
#4 0x608a56 in call_checks /home/christian/Coding/workspaces/selint/src/check_hooks.c:62:9
#5 0x602887 in run_checks_on_one_file /home/christian/Coding/workspaces/selint/src/runner.c:350:27
#6 0x602c92 in run_all_checks /home/christian/Coding/workspaces/selint/src/runner.c:391:4

I tried running that with valgrind I couldn't get that kind of output

@cgzones
Copy link
Contributor

cgzones commented Dec 15, 2020

./configure CC=clang-11 CFLAGS="-O1 -g -fsanitize=address -fsanitize-address-use-after-scope -fno-omit-frame-pointer -fsanitize=undefined -fsanitize=nullability -fsanitize=implicit-conversion -fsanitize=integer -fsanitize=float-divide-by-zero -fsanitize=local-bounds"
make
cd tests/functional/ && bats end-to-end.bats

@gdesrosi
Copy link
Contributor Author

Thanks @cgzones

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants